-
Notifications
You must be signed in to change notification settings - Fork 21
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Upsample phantoms #319
Upsample phantoms #319
Conversation
…nted for brain_phantom2D, brain_phantom3D and pelvis_phantom2D.2 modified: Phantom.jl
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you change
function pelvis_phantom2D(; ss=4)
to
function pelvis_phantom2D(; ss=4, us=1)
Yes, the CI test says that the variable |
Hmmm as the parameters |
modified: Phantom.jl
@aTrotier @beorostica
I think I read the comment differently, below original reply was to somehow calculate the needed resolution to avoid aliasing. Having the phantom at a particular resolution is tricky as it is still locked into multiples or integer fractions of the particular source data. #319 (comment) This is a good point...it is complicated though. It seems like Cartesian GRE sequences see the phantom issue the most. Radial (GRE or UTE) also sees it, sometimes even worse. Spiral and EPI with long echo trains have many less issues. One possibility is to put a wrapper around them, if possible, as a helper function. It would take the seq file or some other explicit parameters as arguments? I'm thinking that the simulation functions are probably the place to do the calculation, as all the needed arguments are already present and that is where the problem could occur. It could then give a warning if a possibility of undersampling exists. Also a note in the phantom and/or simulation docstrings? The idea is to leave things as flexible as possible but giving fair warning when a phantom/simulation interaction could occur? |
Would it be reasonable to include an @info output for the phantom resolution in [mm]? Also include the phantom voxel size in the structure? This would not break any current behavior. |
…antom3. example: julia> obj= pelvis_phantom2D(;); [ Info: Pelvis phantom 2D sample spacing is 2.0 mm. modified: Phantom.jl
modified: Phantom.jl
Yeah, it is tricky. Let's leave it like this while we think of a better way.
I would like to reduce the number of messages being displayed in the REPL, or at least the ones that happen when opening the UI. We could add it to the "name" of the We should definitely need to report the base resolution of the
That is not a bad idea, we could have a dictionary where we can put relevant information for the phantom ( I would revert the |
It could be a warning related to the Cartesian sampling matrix and phantom resolution at some point.
Sounds good! I'll revert the info message. I'll open the obj.info structure issue as new feature, and can work on it. |
…elvis_phantom3." This reverts commit e899238. modified: Phantom.jl
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will merge this, and we can continue in #322.
Upsample phantoms, example:
obj_ui[] = brain_phantom2D(;us=3)
implemented for brain_phantom2D, brain_phantom3D and pelvis_phantom2D.modified: Phantom.jl